Implementing `gemmi`-based mmcif reader (with easy extension to PDB/PDBx and mmJSON) by marinegor · Pull Request #4712 · MDAnalysis/mdanalysis

marinegor · 2024-09-20T21:57:20Z

Fixes #2367 and also extends #4303 and solves #5089

Changes made in this Pull Request:

uses gemmi library (link) to parse mmcif files
adds a class MMCIFReader(base.SingleFrameReaderBase) and class MMCIFParser(TopologyReaderBase) classes for that

As a bonus, this implementation would potentially allow to read any of the gemmi-supported formats (source):

mmCIF (PDBx/mmCIF),
PDB (with popular extensions),
mmJSON

Also, this (with slight modifications) also would allow reading mmcif with multiple models sharing the same topology, as well as more feature-rich parsing of PDBs (the same code without changes can be used for parsing altlocs, charges, etc, from all of these formats).

However, I'm slightly lost on what's to be done next for this PR to be merged, so I'm asking if someone could help me navigate here (tagging @richardjgowers here as author of original PDBx implementation 4303).

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

📚 Documentation preview 📚: https://mdanalysis--4712.org.readthedocs.build/en/4712/

pep8speaks · 2024-09-20T21:57:28Z

Hello @marinegor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/coordinates/MMCIF.py:

Line 28:80: E501 line too long (84 > 79 characters)
Line 41:80: E501 line too long (85 > 79 characters)
Line 42:80: E501 line too long (93 > 79 characters)
Line 61:80: E501 line too long (104 > 79 characters)
Line 65:80: E501 line too long (87 > 79 characters)
Line 67:80: E501 line too long (107 > 79 characters)

In the file package/MDAnalysis/topology/MMCIFParser.py:

Line 2:24: W291 trailing whitespace
Line 60:80: E501 line too long (111 > 79 characters)
Line 72:80: E501 line too long (123 > 79 characters)
Line 82:80: E501 line too long (122 > 79 characters)
Line 106:80: E501 line too long (108 > 79 characters)
Line 113:80: E501 line too long (80 > 79 characters)
Line 128:80: E501 line too long (91 > 79 characters)
Line 175:80: E501 line too long (126 > 79 characters)
Line 185:80: E501 line too long (125 > 79 characters)
Line 224:80: E501 line too long (126 > 79 characters)
Line 242:80: E501 line too long (140 > 79 characters)
Line 281:80: E501 line too long (87 > 79 characters)
Line 292:80: E501 line too long (90 > 79 characters)

In the file package/MDAnalysis/topology/PDBParser.py:

Line 56:80: E501 line too long (80 > 79 characters)
Line 57:80: E501 line too long (84 > 79 characters)

In the file package/MDAnalysis/topology/__init__.py:

Line 335:26: W292 no newline at end of file

In the file testsuite/MDAnalysisTests/datafiles.py:

Line 48:80: E501 line too long (103 > 79 characters)
Line 81:80: E501 line too long (80 > 79 characters)
Line 97:80: E501 line too long (86 > 79 characters)
Line 271:80: E501 line too long (90 > 79 characters)
Line 340:80: E501 line too long (104 > 79 characters)
Line 387:80: E501 line too long (83 > 79 characters)
Line 436:80: E501 line too long (80 > 79 characters)
Line 463:80: E501 line too long (80 > 79 characters)
Line 481:80: E501 line too long (80 > 79 characters)
Line 493:80: E501 line too long (80 > 79 characters)
Line 494:80: E501 line too long (80 > 79 characters)
Line 497:80: E501 line too long (83 > 79 characters)
Line 498:80: E501 line too long (86 > 79 characters)
Line 546:80: E501 line too long (82 > 79 characters)
Line 547:80: E501 line too long (82 > 79 characters)
Line 549:80: E501 line too long (88 > 79 characters)
Line 551:80: E501 line too long (88 > 79 characters)
Line 552:80: E501 line too long (81 > 79 characters)
Line 777:80: E501 line too long (81 > 79 characters)
Line 778:80: E501 line too long (87 > 79 characters)
Line 779:80: E501 line too long (84 > 79 characters)
Line 780:80: E501 line too long (85 > 79 characters)
Line 781:80: E501 line too long (83 > 79 characters)

Comment last updated at 2024-10-25 11:17:29 UTC

github-actions · 2024-09-20T21:59:32Z

Linter Bot Results:

Hi @marinegor! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/11148966346/job/30986736623

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

richardjgowers

Looks good so far, will require a small test file to check reader/parser halves.

BradyAJohnston

I've gone through and addressed tweaks and cleanup.

I did a minor refactor of the atom loop into something a bit more readable.

Should be ready to go now!

orbeckst

I only have minor doc fix/change requests. Looks good to go from the technical side. There are some explicit python loops that look slow but as the comments say, check back when the gemmi Python API gives access to flat structures.

orbeckst

Thanks for addressing my comments; all good from my side.

(I left optional suggestions, which are really flavor-to-taste, whatever you think works better.)

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

BradyAJohnston · 2026-05-26T11:30:42Z

@IAlibay If we are are headed for a new release, would love to get this PR merged & part of if if you could take a final look.

ianmkenney · 2026-06-17T12:52:36Z

+1 on seeing this merged into the next release!

orbeckst · 2026-07-12T00:06:43Z

@IAlibay your comment had been addressed and the gemmi reader is also tested in Windows on azure. Could you updated your review, please? Otherwise I'll dismiss it in a day in an effort to merge this important new feature.

(I am not sure what the linter wants, I'll try to make it happy.)

IAlibay · 2026-07-12T03:04:58Z

@IAlibay your comment had been addressed and the gemmi reader is also tested in Windows on azure. Could you updated your review, please? Otherwise I'll dismiss it in a day in an effort to merge this important new feature.

(I am not sure what the linter wants, I'll try to make it happy.)

@orbeckst can you give me until Tuesday please? I agree it's an important feature and I do want to review it, however there's some other high priority items within MDAnalysis that needs addressing first.

Either way, I'm not going to be releasing a be version of MDAnalysis until after the 17th - so it not being merged until then isn't an issue.

orbeckst · 2026-07-12T16:14:47Z

Ok

marinegor added 13 commits May 22, 2024 20:04

Start working on MMCIF parser

aa2a88f

Add first (not working) version of MMCIFReader and MMCIF topology parser

218cf43

Do some squashing

7f78e02

Remove inherited docs

6682d6e

Try improving the parsing

817f3a0

Try three independent loops over the model

3cc8c80

Merge remote-tracking branch 'upstream/develop' into feature/mmcif

f1bf325

Add gemmi dependency

d21c220

necessary params

2a1be15

finished sorting atom attrs

77645e6

add function for transformation into *idx

91e6942

oh damn seems to finally be working

9a0c086

remove TODOs

9c731df

Remove debug prints

8b40ec7

richardjgowers reviewed Sep 21, 2024

View reviewed changes

marinegor added 13 commits September 23, 2024 00:31

Merge branch 'develop' into feature/mmcif

bdcbd73

try to pack things into separate class in utils?

401a4d3

remove unnecessary functions

9c336bd

copy all loops into separate functions

def88e4

Move loops over structures into functions

cabfd37

Move coordinate fetching into function for the coordinate reader as well

4c9d930

Fix imports

184491a

Start adding documentation

3de8565

Reference MMCIFParser in PDBParser

ca6ebbb

Add documentation for trajectory and topology parsers

45077ad

Add mmcif tests

9a1a59a

Update format specifications

27c10d6

Write simple tests

950cfcf

BradyAJohnston added 6 commits April 27, 2026 18:04

de-duplicate _read_gemmi_structure

2b64581

properly expose parsers in topology/init.py

d1d8467

undo unnecessary tpr changes

bb857a8

refactor main zip into simpler loop

b026179

match case & logging

547eed7

fix gemmi docs linking

95600b1

BradyAJohnston approved these changes Apr 28, 2026

View reviewed changes

BradyAJohnston requested a review from orbeckst April 28, 2026 08:11

BradyAJohnston added 2 commits April 28, 2026 09:16

import Structure behind TYPE_CHECKING

e053cd6

fix type imports

a7ea152

orbeckst requested changes Apr 29, 2026

View reviewed changes

BradyAJohnston added 2 commits April 29, 2026 15:16

cleanup / fix docs

40d7747

black format

6d4d9a4

orbeckst approved these changes Apr 29, 2026

View reviewed changes

Comment thread package/MDAnalysis/coordinates/MMCIF.py Outdated

Comment thread package/MDAnalysis/topology/MMCIFParser.py Outdated

Apply suggestions from code review

4879d6b

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

IAlibay added this to the 2.11 milestone May 27, 2026

orbeckst mentioned this pull request May 30, 2026

REL: release 2.11.0 #5390

Open

8 tasks

Merge branch 'develop' into feature/mmcif

39bd465

orbeckst assigned orbeckst and unassigned richardjgowers and hmacdope Jul 12, 2026

orbeckst added 4 commits July 11, 2026 17:20

reformatted with black 24.10

5ec4a33

blacken doc conf.py with local black settings

f734ec3

blacken test files with local pyproject.toml settings

b592d34

blackened another file to make black/linter happy

8751da6

Uh oh!

Conversation

marinegor commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers certificate of origin

Uh oh!

pep8speaks commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2024-10-25 11:17:29 UTC

Uh oh!

github-actions Bot commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linter Bot Results:

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BradyAJohnston left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BradyAJohnston commented May 26, 2026

Uh oh!

ianmkenney commented Jun 17, 2026

Uh oh!

orbeckst commented Jul 12, 2026

Uh oh!

IAlibay commented Jul 12, 2026

Uh oh!

orbeckst commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

marinegor commented Sep 20, 2024 •

edited

Loading

pep8speaks commented Sep 20, 2024 •

edited

Loading

github-actions Bot commented Sep 20, 2024 •

edited

Loading